Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fixed wrapped inputs with non-string values & checkboxes #129

Merged
merged 2 commits into from
Aug 17, 2016
Merged

Fixed wrapped inputs with non-string values & checkboxes #129

merged 2 commits into from
Aug 17, 2016

Conversation

tonsky
Copy link
Contributor

@tonsky tonsky commented Aug 16, 2016

First, 579f3d7 introduced a bug when inputs which have non-string :value were unable to update. Two people reported that, I hit that too, and that patch resolves that by coercing props.value to string before storing it into state_value.

Second, I just realized that wrapped-form-element won’t work properly for checkboxes and radio buttons, because they use checked instead of value. But they have value as well, it just mean different thing for them. So there’s new branch for wrapping checkboxes/radio buttons now

@r0man
Copy link
Owner

r0man commented Aug 17, 2016

@tonsky The tests are failing. Also, wrap-value-input and wrap-checked-input are pretty similar. Could you please remove this duplication somehow? Either using helper fns for the individual method implementations or an additional argument to wrap-form-element. Something like this:

(defn wrap-form-element  [element attribute] ...)
(wrap-form-element "input" :value)
(wrap-form-element "checkbox" :checked)

I think I would prefer the second approach. What do you think?
In any case, thanks for fixing this!

@tonsky
Copy link
Contributor Author

tonsky commented Aug 17, 2016

Yeah, I’ve seen the tests. Turned out we were still using wrapped inputs for uncontrolled components. When I fix that, it turned out that we can’t use native inputs because of value=nil in props :(

@tonsky
Copy link
Contributor Author

tonsky commented Aug 17, 2016

Ok, I think I’ve fixed that. Tests pass, I returned single wrap-form-element and simplified create-element a little

@r0man
Copy link
Owner

r0man commented Aug 17, 2016

@tonsky Thanks! Let me check it on one of my projects, then I'll merge it and cut a release.

@r0man r0man merged commit 905ce9b into r0man:master Aug 17, 2016
@r0man
Copy link
Owner

r0man commented Aug 17, 2016

@tonsky Ok, released as 0.7.4. Thanks again.

@tonsky
Copy link
Contributor Author

tonsky commented Aug 17, 2016

Awesome! Thanks a lot

On Wed, Aug 17, 2016 at 3:48 PM r0man [email protected] wrote:

@tonsky https://github.com/tonsky Ok, released as 0.7.4. Thanks again.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#129 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AARabHcJJLMeS9ZyHLhtRzA94cdUmL6tks5qgxE-gaJpZM4JlppD
.

@tonsky
Copy link
Contributor Author

tonsky commented Aug 20, 2016

Hey, I noticed you’re from Berlin, right? Wanna meet tomorrow? I’ll be there https://twitter.com/nikitonsky/status/766897501534162944

@r0man
Copy link
Owner

r0man commented Aug 22, 2016

Hey Nikita,
I'm on a surf trip through France and Spain at the moment. Next time!
Have fun in Berlin.
Roman

On 20 Aug 2016 09:25, "Nikita Prokopov" [email protected] wrote:

Hey, I noticed you’re from Berlin, right? Wanna meet tomorrow? I’ll be
there https://twitter.com/nikitonsky/status/766897501534162944

—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
#129 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AABUPkuNx0vQUhXadVtCnwgQql1rUrt6ks5qhqvegaJpZM4JlppD
.

@tonsky tonsky mentioned this pull request Apr 7, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@tonsky @r0man